Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config/v1: New Release type for ClusterVersionStatus #521

Merged

Conversation

wking
Copy link
Member

@wking wking commented Nov 19, 2019

This commit adds the ability to store release metadata, for example the list of channels that a release is in:

$ curl -sH 'Accept:application/json' 'https://api.openshift.com/api/upgrades_info/v1/graph?channel=stable-4.2' | jq -r '.nodes[] | select(.version == "4.2.4").metadata["io.openshift.upgrades.graph.release.channels"] | split(",")[]'
candidate-4.2
fast-4.2
stable-4.2

That will allow the console and other downstream tooling to expose a list of channels available to a given cluster right now, instead of hard-coding an expected list. This will account for things like phased releases, where we may not recommend an upgrade for your particular stable-4.2 cluster even though we are recommending the upgrade for other stable-4.2 clusters.

Pivoting around this retyping in Go will require consumer bumps, but I don't think it's a breaking change because the only YAML removal is force, which is meaningless in AvailableUpdates. So I don't think this needs to count as a backwards-compat-breaking schema bump.

Some previous discussion in openshift/cincinnati#171.

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 19, 2019
@abhinavdahiya
Copy link
Contributor

needs an enhancement.

@wking
Copy link
Member Author

wking commented Nov 19, 2019

needs an enhancement.

Filed openshift/enhancements#123

@wking wking changed the title config/v1: New Release type for ClusterVersionStatus.AvailableUpdates config/v1: New Release type for ClusterVersionStatus Nov 19, 2019
@wking
Copy link
Member Author

wking commented Nov 19, 2019

Pushed c8bff82 -> e468717 to move ClusterVersionStatus.Desired over too, as laid out in the enhancement proposal.

@smarterclayton
Copy link
Contributor

/hold

for enhancement discussion

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 24, 2019
@wking wking force-pushed the available-release-metadata branch from e468717 to ae27521 Compare June 4, 2020 20:05
@wking
Copy link
Member Author

wking commented Jun 4, 2020

Rebased onto master and updated to pull in the Release type from openshift/enhancements@9e9e0f511939f1e3 with e468717 -> ae27521.

@wking
Copy link
Member Author

wking commented Jul 21, 2020

Enhancement landed. Will reroll shortly.

@wking wking force-pushed the available-release-metadata branch from ae27521 to d5c4096 Compare July 21, 2020 15:45
@wking
Copy link
Member Author

wking commented Jul 21, 2020

Updated to match the landed openshift/enhancements#123 with ae27521 -> d5c4096.

Copy link
Member

@LalatenduMohanty LalatenduMohanty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2020
@wking
Copy link
Member Author

wking commented Jul 21, 2020

/hold cancel

Held for the enhancement, which has landed.

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 21, 2020
@smarterclayton
Copy link
Contributor

/hold

Until the text is updated

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 21, 2020
wking added a commit to wking/openshift-enhancements that referenced this pull request Jul 21, 2020
Fixups to 2eb16cf (enhancements/update/available-update-metadata:
Propose a new enhancement, 2019-11-19, openshift#123):

* Including two sentences from Clayton about how the CVO sources this
  information and how we build CI/nightly images [1].
* Downcasing in the Go comments, to match the JSON property, following
  the Kubernetes API pattern.
* 'despire' -> 'despite' typo fix.

[1]: openshift/api#521 (comment)
wking added a commit to wking/openshift-enhancements that referenced this pull request Jul 21, 2020
Fixups to 2eb16cf (enhancements/update/available-update-metadata:
Propose a new enhancement, 2019-11-19, openshift#123):

* Including two sentences from Clayton about how the CVO sources this
  information and how we build CI/nightly images [1].

  I think these two sentences are poking holes in the abstraction that
  the CVO is committing to with the new API, and that consumers should
  not have to know or care about these implementation details.
  Clayton feels like they provide useful context about how the system
  works, and that we'll adjust them as necessary if we ever have cause
  to alter the implementation.

* Downcasing in the Go comments, to match the JSON property, following
  the Kubernetes API pattern.

* 'despire' -> 'despite' typo fix.

[1]: openshift/api#521 (comment)
wking added a commit to wking/openshift-enhancements that referenced this pull request Jul 21, 2020
Fixups to 2eb16cf (enhancements/update/available-update-metadata:
Propose a new enhancement, 2019-11-19, openshift#123):

* Including a sentence from Clayton about how we currently `build
  CI/nightly images [1].

  I think this sentence is poking holes in the abstraction that the
  CVO is committing to with the new API, and that consumers should not
  have to know or care about these implementation details.  Clayton
  feels like it provides useful context about how the system works,
  and that we'll adjust them as necessary if we ever have cause to
  alter the implementation.

  I am rejecting his "This URL is set by the 'url' metadata property
  on a release..." suggestion [1], because the enhancement proposal
  includes:

    Then, in the cluster-version operator (CVO), this enhancement
    proposes porting existing logic like available-update translation
    and available-update lookup to preserve the Cincinnati metadata,
    with information from the release image itself taking precedence
    over information from Cincinnati.

  That information will populate url from the release image when the
  release image declares a url (always, for official OpenShift
  releases), but fall back to Cincinnati if it is not set in the
  release.  Clayton's sentence does not talk about the Cincinnati
  fallback possibility.

  I am rejecting his "...should be displayed as a link in user
  interfaces" because interfaces that want to display a link will
  already know that this is their most-generic choice based on the
  other docstring sentences.  But I'm fine including this fragment if
  folks feel like it is adding something useful on its own, with the
  "metadata property on a release" portion removed.

* Downcasing in the Go comments, to match the JSON property, following
  the Kubernetes API pattern.

* 'despire' -> 'despite' typo fix.

[1]: openshift/api#521 (comment)
wking added a commit to wking/openshift-enhancements that referenced this pull request Jul 21, 2020
Fixups to 2eb16cf (enhancements/update/available-update-metadata:
Propose a new enhancement, 2019-11-19, openshift#123):

* Including two sentences from Clayton about how the CVO sources this
  information and how we build CI/nightly images [1,2].

  I think these two sentences are poking holes in the abstraction that
  the CVO is committing to with the new API, and that consumers should
  not have to know or care about these implementation details.
  Clayton feels like they provide useful context about how the system
  works, and that we'll adjust them as necessary if we ever have cause
  to alter the implementation.

* Downcasing in the Go comments, to match the JSON property, following
  the Kubernetes API pattern.

* 'despire' -> 'despite' typo fix.

[1]: openshift/api#521 (comment)
[2]: openshift/api#521 (comment)
wking added a commit to wking/openshift-enhancements that referenced this pull request Jul 24, 2020
Fixups to 2eb16cf (enhancements/update/available-update-metadata:
Propose a new enhancement, 2019-11-19, openshift#123):

* Including two sentences from Clayton about how the CVO sources this
  information and how we build CI/nightly images [1,2].

  I think these two sentences are poking holes in the abstraction that
  the CVO is committing to with the new API, and that consumers should
  not have to know or care about these implementation details.
  Clayton feels like they provide useful context about how the system
  works, and that we'll adjust them as necessary if we ever have cause
  to alter the implementation.

* Downcasing in the Go comments, to match the JSON property, following
  the Kubernetes API pattern.

* 'despire' -> 'despite' typo fix.

[1]: openshift/api#521 (comment)
[2]: openshift/api#521 (comment)
wking added a commit to wking/openshift-enhancements that referenced this pull request Jul 24, 2020
Fixups to 2eb16cf (enhancements/update/available-update-metadata:
Propose a new enhancement, 2019-11-19, openshift#123):

* Including two sentences from Clayton about how the CVO sources this
  information and how we build CI/nightly images [1,2].

  I think these two sentences are poking holes in the abstraction that
  the CVO is committing to with the new API, and that consumers should
  not have to know or care about these implementation details.
  Clayton feels like they provide useful context about how the system
  works, and that we'll adjust them as necessary if we ever have cause
  to alter the implementation.

* Downcasing in the Go comments, to match the JSON property, following
  the Kubernetes API pattern.

* 'despire' -> 'despite' typo fix.

[1]: openshift/api#521 (comment)
[2]: openshift/api#521 (comment)
wking added a commit to wking/openshift-enhancements that referenced this pull request Jul 24, 2020
Fixups to 2eb16cf (enhancements/update/available-update-metadata:
Propose a new enhancement, 2019-11-19, openshift#123):

* Including two sentences from Clayton about how the CVO sources this
  information and how we build CI/nightly images [1,2].

  I think these two sentences are poking holes in the abstraction that
  the CVO is committing to with the new API, and that consumers should
  not have to know or care about these implementation details.
  Clayton feels like they provide useful context about how the system
  works, and that we'll adjust them as necessary if we ever have cause
  to alter the implementation.

* Downcasing in the Go comments, to match the JSON property, following
  the Kubernetes API pattern.

* 'despire' -> 'despite' typo fix.

[1]: openshift/api#521 (comment)
[2]: openshift/api#521 (comment)
wking added a commit to wking/openshift-enhancements that referenced this pull request Jul 24, 2020
Fixups to 2eb16cf (enhancements/update/available-update-metadata:
Propose a new enhancement, 2019-11-19, openshift#123):

* Including two sentences from Clayton about how the CVO sources this
  information and how we build CI/nightly images [1,2].

  I think these two sentences are poking holes in the abstraction that
  the CVO is committing to with the new API, and that consumers should
  not have to know or care about these implementation details.
  Clayton feels like they provide useful context about how the system
  works, and that we'll adjust them as necessary if we ever have cause
  to alter the implementation.

* Downcasing in the Go comments, to match the JSON property, following
  the Kubernetes API pattern.

* 'despire' -> 'despite' typo fix.

[1]: openshift/api#521 (comment)
[2]: openshift/api#521 (comment)
This commit adds the ability to store release metadata, implementing
openshift/enhancements@2eb16cf80d
(enhancements/update/available-update-metadata: Propose a new
enhancement, 2020-07-20, openshift/enhancements#123) as adjusted by
openshift/enhancements@12eff48079
(enhancements/update/available-update-metadata: 'url' docstring
context, 2020-07-21, openshift/enhancements#408).  For example the
list of channels that a release is in:

  $ curl -sH 'Accept:application/json' 'https://api.openshift.com/api/upgrades_info/v1/graph?channel=stable-4.2' | jq -r '.nodes[] | select(.version == "4.2.4").metadata["io.openshift.upgrades.graph.release.channels"] | split(",")[]'
  candidate-4.2
  fast-4.2
  stable-4.2

That will allow the console and other downstream tooling to expose a
list of channels available to a given cluster right now, instead of
hard-coding an expected list [1].  This will account for things like
phased releases, where we may not recommend an upgrade for your
particular stable-4.2 cluster even though we are recommending the
upgrade for other stable-4.2 clusters.

Pivoting around this retyping in Go will require consumer bumps, but I
don't think it's a breaking change because the only YAML removal is
'force', which is meaningless in AvailableUpdates.  So I don't think
this needs to count as a backwards-compat-breaking schema bump.

Autogenerated bumps via:

  $ unset GOBIN  # vendor/k8s.io/code-generator/generate-groups.sh and such require "${GOPATH}/bin/deepcopy-gen"
  $ go get k8s.io/gengo/examples/deepcopy-gen
  $ hack/update-deepcopy.sh
  $ hack/update-swagger-docs.sh
  $ make update-codegen-crds

[1]: openshift/console#2935
@wking wking force-pushed the available-release-metadata branch from d5c4096 to 575f8d2 Compare July 24, 2020 20:31
@openshift-ci-robot openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 24, 2020
@smarterclayton
Copy link
Contributor

/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LalatenduMohanty, smarterclayton, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 24, 2020
@openshift-merge-robot openshift-merge-robot merged commit 3ae6754 into openshift:master Jul 24, 2020
@wking wking deleted the available-release-metadata branch August 3, 2020 22:38
damemi added a commit to damemi/oc that referenced this pull request Sep 2, 2020
This reacts to the changes in openshift/api#521, which
introduced a new configv1.Release type in the CVO.
damemi added a commit to damemi/oc that referenced this pull request Sep 2, 2020
This reacts to the changes in openshift/api#521, which
introduced a new configv1.Release type in the CVO.
damemi added a commit to damemi/oc that referenced this pull request Sep 2, 2020
This reacts to the changes in openshift/api#521, which
introduced a new configv1.Release type in the CVO.
damemi added a commit to damemi/oc that referenced this pull request Sep 8, 2020
This reacts to the changes in openshift/api#521, which
introduced a new configv1.Release type in the CVO.
wking added a commit to wking/openshift-api that referenced this pull request Aug 17, 2021
This type used to be used for both the admin-provided
spec.desiredUpdate and the operator-provided status.desired.  That
changed in 575f8d2 (config/v1: New Release type for
ClusterVersionStatus, 2019-01-19, openshift#521, 4.6), when status.desired
moved to a new Release type, so now Update is only used for
spec.desiredUpdate.

The properties have been '+optional' since they landed in 898d7e3
(api: Move ClusterVersion/ClusterOperator into config.openshift.io,
2018-11-09, openshift#127) and ab4ff93 (Update ClusterVersion to have a
'force' update flag and track verified, 2019-04-22, openshift#293).

I'm adding 'omitempty', because if the admin doesn't have a particular
version or image in mind, returning explicit empty strings is
distracting noise.  With this commit, it will be easier to focus on
the version or image property that did get set, and you need at least
one of them to be set to be a usable update request.

The force property is a bit more wiggly, since there may be some
benefit to explicitly pointing out that the admin is not forcing the
update.  But forcing is supposed to be an exceptional-situation safety
valve, so I'm adding omitempty there too, because the benefit of
de-emphasizing the property's presence (and reducing the chance that
an admin says "hey, let's see what 'force: true' does" without reading
associated docs) outweighs the benefit of explicitly pointing out
'force: false' cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants